-
Notifications
You must be signed in to change notification settings - Fork 390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(core): bump messageformat parser. Add TS types #1328
refactor(core): bump messageformat parser. Add TS types #1328
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@thekip thanks for the contribution! Probably, I'll ask @Martin005 to review this PR.
It expects that the PR title will be specified according to the following format: https://github.com/amannn/action-semantic-pull-request#validation |
…bump message-parser
size-limit report 📦
|
My bad, didn't check this. This commit is bigger, few things about what changed:
Before compiled catalogs were produced as js template literal (js code) and complicated AST manipulation was involved export const messages = {
messageId: "message value"
} Now i'm just using JSON.stringify / JSON.parse export const messages = JSON.parse("{
"messageId": "message value"
}") Why it's better you can read here https://v8.dev/blog/cost-of-javascript-2019#json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes, I think that all of what you mentioned makes perfect sense including the JSON.stringify/JSON.parse! 🙂
When I tried your branch as a local package, I immediately noticed one mistake, so please have a look at my comments and resolve them.
When I resolved the mistake locally, I also tried all 4 issues #1033, #1075, #1278 and #1303 and I can confirm that all of them are fixed with this new version of @messageformat/parser
package 🥳
@@ -28,6 +28,7 @@ function remoteLoader<T>({ format = "minimal", fallbackMessages, messages}: Remo | |||
`) | |||
} | |||
|
|||
// todo: that will not work with context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This todo is here because the code in codebase already not working with context and changing this package was out of scope of that PR.
Fixed |
I also thinking on how to hide exposed but treated as internal APIs, for example that There should be either special naming convention, or may be just JSdoc would be enough |
I see tests are failing, i will have a look |
I've ported some changes from this PR #1328
And also i've ported tests, because previous ones didn't test anything (there was a logical issue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you for the changes!
Bumped version of the upstream messageformat parser to 5.0, updated what's broke and add TS typings wherever possible in touched files.
Resolves
#1278
#1303
Should be no changes in logic, but public types slightly changed. They became more accurate, so it's better to bump a version just in case.
How current config of semver works, should i name a PR with special prefix or commit message or it's doing manually?